Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor(linter): refactor LintBuilder to prep for nested configs #8034

Merged
merged 1 commit into from
Jan 5, 2025
Merged

refactor(linter): refactor LintBuilder to prep for nested configs #8034

merged 1 commit into from
Jan 5, 2025
+122 −124

Conversation

camc314
Copy link
Contributor

@camc314 camc314 commented Dec 19, 2024 •
edited
Loading

More simplification/preparations for nested configurations:

  1. renames LinterBuilder to ConfigStoreBuilder
  2. moves the options logic out of LintBuilder
  3. make ConfigStoreBuilder::build() return a result (currently always returns OK, but it will return errors when nested config is implemented

The next steps to implement nested config which i hope to do in the next week are:

  1. refactor the from_oxlintrc to accept a file path
  2. introduce a new method on ConfigStoreBuilder (name TBC) that walks all child directories, collecting .oxlintrc files. these will be put into ConfigStore as a hash map of path > config.

This was referenced Dec 19, 2024
Merged
Merged
@camc314 Graphite App
Copy link
Contributor Author

camc314 commented Dec 19, 2024 •
edited
Loading

How to use the Graphite Merge Queue

Add either label to this PR to merge it via the merge queue:

  • 0-merge - adds this PR to the back of the merge queue
  • hotfix - for urgent hot fixes, skip the queue and merge this PR next

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

This stack of pull requests is managed by Graphite. Learn more about stacking.

@github-actions github-actions bot added A-linter Area - Linter A-cli Area - CLI A-editor Area - Editor and Language Server C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior labels Dec 19, 2024
@camc314 camc314 marked this pull request as ready for review December 19, 2024 22:45
@camc314 camc314 force-pushed the c/12-19-refactor_linter_renamce_flat.rs_to_config.rs_ branch from 73d39ed to f6523f6 Compare December 19, 2024 22:54
@camc314 camc314 force-pushed the c/12-19-refactor_linter_refactor_lintbuilder_to_prep_for_nested_configs branch from ae07fda to 42adf86 Compare December 19, 2024 22:54
@Boshen Boshen requested a review from camchenry December 20, 2024 02:38
@camchenry
Copy link
Contributor

Thanks for taking a look at this, I'll review this as soon as possible. I had a draft of the nested config work on my laptop but wasn't happy with it so far.

@camc314
Copy link
Contributor Author

camc314 commented Dec 20, 2024

Thanks for taking a look at this, I'll review this as soon as possible. I had a draft of the nested config work on my laptop but wasn't happy with it so far.

ah sorry! i didn't realize you had already started implementing this.
feel free to close this one in favour of yours!

camchenry
camchenry approved these changes Dec 20, 2024
View reviewed changes
Copy link
Contributor

@camchenry camchenry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

crates/oxc_linter/src/config/config_builder.rs Outdated Show resolved Hide resolved
@camc314 camc314 force-pushed the c/12-19-refactor_linter_renamce_flat.rs_to_config.rs_ branch from f6523f6 to 0487198 Compare December 20, 2024 19:53
@camc314 camc314 force-pushed the c/12-19-refactor_linter_refactor_lintbuilder_to_prep_for_nested_configs branch 3 times, most recently from 30c712c to 0eeb0e0 Compare December 20, 2024 19:56
@Boshen
Copy link
Member

Boshen commented Dec 21, 2024

You may merge after fixing the compile error.

@Boshen Boshen changed the base branch from c/12-19-refactor_linter_renamce_flat.rs_to_config.rs_ to graphite-base/8034 December 21, 2024 02:02
@Boshen Boshen force-pushed the c/12-19-refactor_linter_refactor_lintbuilder_to_prep_for_nested_configs branch from 0eeb0e0 to e46b6e7 Compare December 21, 2024 02:08
@Boshen Boshen force-pushed the graphite-base/8034 branch from 0487198 to 952d7e4 Compare December 21, 2024 02:08
@Boshen Boshen changed the base branch from graphite-base/8034 to main December 21, 2024 02:09
@Boshen Boshen force-pushed the c/12-19-refactor_linter_refactor_lintbuilder_to_prep_for_nested_configs branch from e46b6e7 to 263abe6 Compare December 21, 2024 02:09
@camchenry camchenry force-pushed the c/12-19-refactor_linter_refactor_lintbuilder_to_prep_for_nested_configs branch from 263abe6 to 5c25148 Compare January 4, 2025 19:50
@codspeed-hq CodSpeed HQ
Copy link

codspeed-hq bot commented Jan 4, 2025 •
edited
Loading

CodSpeed Performance Report

Merging #8034 will not alter performance

Comparing c/12-19-refactor_linter_refactor_lintbuilder_to_prep_for_nested_configs (3c534ae) with main (65f46f5)

Summary

✅ 29 untouched benchmarks

@camchenry camchenry force-pushed the c/12-19-refactor_linter_refactor_lintbuilder_to_prep_for_nested_configs branch from 5c25148 to ad6ab1d Compare January 4, 2025 20:39
@camchenry
Copy link
Contributor

@camc314 I pushed some changes to fix errors where we were using Linter::default previously. I updated it to include the config building as well with the default LintOptions for the Linter too.

@Boshen Boshen added the 0-merge Merge with Graphite Merge Queue label Jan 5, 2025
@Boshen Graphite App
Copy link
Member

Boshen commented Jan 5, 2025 •
edited by graphite-app bot
Loading

Merge activity

  • Jan 4, 11:07 PM EST: The merge label '0-merge' was detected. This PR will be added to the Graphite merge queue once it meets the requirements.
  • Jan 4, 11:07 PM EST: A user added this pull request to the Graphite merge queue.
  • Jan 4, 11:13 PM EST: A user merged this pull request with the Graphite merge queue.

@camc314
refactor(linter): refactor LintBuilder to prep for nested configs (#…
3c534ae 
…8034)

More simplification/preparations for nested configurations:

1. renames `LinterBuilder` to `ConfigStoreBuilder`
2. moves the `options` logic out of `LintBuilder`
3. make `ConfigStoreBuilder::build()` return a result (currently always returns OK, but it will return errors when nested config is implemented

The next steps to implement nested config which i hope to do in the next week are:
1. refactor the `from_oxlintrc` to accept a file path
2. introduce a new method on `ConfigStoreBuilder` (name TBC) that walks all child directories, collecting `.oxlintrc` files. these will be put into `ConfigStore` as a hash map of path > config.
@Boshen Boshen force-pushed the c/12-19-refactor_linter_refactor_lintbuilder_to_prep_for_nested_configs branch from ad6ab1d to 3c534ae Compare January 5, 2025 04:08
@graphite-app graphite-app bot merged commit 3c534ae into main Jan 5, 2025
29 checks passed
@graphite-app graphite-app bot deleted the c/12-19-refactor_linter_refactor_lintbuilder_to_prep_for_nested_configs branch January 5, 2025 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camchenry camchenry camchenry approved these changes

Assignees

@camc314 camc314

Labels
0-merge Merge with Graphite Merge Queue A-cli Area - CLI A-editor Area - Editor and Language Server A-linter Area - Linter C-cleanup Category - technical debt or refactoring. Solution not expected to change behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@camc314 @camchenry @Boshen